[25.1] Use workflow-style payload in data landing request#21107
[25.1] Use workflow-style payload in data landing request#21107mvdbeek merged 2 commits intogalaxyproject:release_25.1from
Conversation
758b3e6 to
d47b5bd
Compare
2748637 to
1de38e5
Compare
|
We have an upload API and we have an API for supplying files to a workflow form - in the abstract you don't think the data landing API should reflect the upload API instead over the workflow run API ? That feels like you're letting your current use case override the best design in the abstract. You're doing the integration work and I'll let you make this change and merge it into 25.1 but I don't like it in the abstract - it makes little sense to me we wouldn't model the uploads by default for this API. Is having an request type parameter we could add to both this API and to the data fetch API to allow both of them to be compatible with the other be a compromise that we could both be happy with? If we're saying the workflow form representation is the best - we should have an interface to upload directly with it I would think. |
jmchilton
left a comment
There was a problem hiding this comment.
Like I said if you're doing the integration work - I'm not going to get in the way but I don't love it.
I think we should have just one documented API and the workflow run API is, while less comprehensive, subjectively easier to write and easier to document and validate (that one I think is less subjective?).
That is where I am coming from, however I don't think that we're preventing any of the advanced features to be implemented with the simpler request API? While we aren't supporting attaching files we shouldn't advertise them in the API though, which we currently do.
A separate route for each sound good to me, yes. I do think we want the upload API to also work with the simpler request syntax, but that's probably more work than I would do right now ? |
1de38e5 to
a2d1851
Compare
I guess I should elaborate, this is the fetch data API: - destination: {type: "hdca"}
collection_type: list
name: "my collection"
items:
- src: url
name: "sample1"
url: "base64://eyJ0c3JjIjogInRlc3QifQ=="
ext: "txt"
- src: url
name: sample2
url: "base64://eyJ0c3JjIjogInRlc3QifQ=="
ext: "txt"
- src: url
name: sample3
url: "base64://eyJ0c3JjIjogInRlc3QifQ=="
ext: "txt"This is the workflow run / file / class API: - class: Collection
collection_type: list
name: "my collection"
elements:
- class: File
name: sample1
location: "base64://eyJ0c3JjIjogInRlc3QifQ=="
ext: "txt"
- class: File
name: sample2
location: "base64://eyJ0c3JjIjogInRlc3QifQ=="
ext: "txt"
- class: File
name: sample3
location: "base64://eyJ0c3JjIjogInRlc3QifQ=="
ext: "txt"if you squint they look similar, but the validation works quite differently. The - '{"type": "missing", "loc": ["body", "request_state", 0, "Collection", "elements",
0, "File", "ext"], "msg": "Field required", "input": {"class": "File", "name": "Reference
information", "url": "base64://eyJ0ZXN0IjogInRlc3QifQ==", "extensionn": "txt"}}'
- '{"type": "extra_forbidden", "loc": ["body", "request_state", 0, "Collection", "elements",
0, "File", "extensionn"], "msg": "Extra inputs are not permitted", "input": "txt"}'kind of readable, right ? this is the same typo with the current API: - '{"type": "extra_forbidden", "loc": ["body", "request_state", "targets", 0, "DataElementsTarget",
"items", 0, "tagged-union[FileDataElement,PastedDataElement,UrlDataElement,PathDataElement,ServerDirElement,FtpImportElement,CompositeDataElement]",
"url", "extensionn"], "msg": "Extra inputs are not permitted", "input": "txt"}'
- '{"type": "missing", "loc": ["body", "request_state", "targets", 0, "DataElementsTarget",
"items", 0, "NestedElement", "elements"], "msg": "Field required", "input": {"src":
"url", "name": "Reference information", "url": "base64://eyJ0ZXN0IjogInRlc3QifQ==",
"extensionn": "txt"}}'
- '{"type": "extra_forbidden", "loc": ["body", "request_state", "targets", 0, "DataElementsTarget",
"items", 0, "NestedElement", "src"], "msg": "Extra inputs are not permitted", "input":
"url"}'
- '{"type": "extra_forbidden", "loc": ["body", "request_state", "targets", 0, "DataElementsTarget",
"items", 0, "NestedElement", "url"], "msg": "Extra inputs are not permitted", "input":
"base64://eyJ0ZXN0IjogInRlc3QifQ=="}'
- '{"type": "extra_forbidden", "loc": ["body", "request_state", "targets", 0, "DataElementsTarget",
"items", 0, "NestedElement", "extensionn"], "msg": "Extra inputs are not permitted",
"input": "txt"}'
- '{"type": "literal_error", "loc": ["body", "request_state", "targets", 0, "HdcaDataItemsTarget",
"destination", "type"], "msg": "Input should be ''hdca''", "input": "hdas"}'
- '{"type": "extra_forbidden", "loc": ["body", "request_state", "targets", 0, "HdcaDataItemsTarget",
"items", 0, "tagged-union[FileDataElement,PastedDataElement,UrlDataElement,PathDataElement,ServerDirElement,FtpImportElement,CompositeDataElement]",
"url", "extensionn"], "msg": "Extra inputs are not permitted", "input": "txt"}'
- '{"type": "missing", "loc": ["body", "request_state", "targets", 0, "HdcaDataItemsTarget",
"items", 0, "NestedElement", "elements"], "msg": "Field required", "input": {"src":
"url", "name": "Reference information", "url": "base64://eyJ0ZXN0IjogInRlc3QifQ==",
"extensionn": "txt"}}'
- '{"type": "extra_forbidden", "loc": ["body", "request_state", "targets", 0, "HdcaDataItemsTarget",
"items", 0, "NestedElement", "src"], "msg": "Extra inputs are not permitted", "input":
"url"}'
- '{"type": "extra_forbidden", "loc": ["body", "request_state", "targets", 0, "HdcaDataItemsTarget",
"items", 0, "NestedElement", "url"], "msg": "Extra inputs are not permitted", "input":
"base64://eyJ0ZXN0IjogInRlc3QifQ=="}'
- '{"type": "extra_forbidden", "loc": ["body", "request_state", "targets", 0, "HdcaDataItemsTarget",
"items", 0, "NestedElement", "extensionn"], "msg": "Extra inputs are not permitted",
"input": "txt"}'
- '{"type": "missing", "loc": ["body", "request_state", "targets", 0, "DataElementsFromTarget",
"src"], "msg": "Field required", "input": {"destination": {"type": "hdas"}, "items":
[{"src": "url", "name": "Reference information", "url": "base64://eyJ0ZXN0IjogInRlc3QifQ==",
"extensionn": "txt"}]}}'
- '{"type": "missing", "loc": ["body", "request_state", "targets", 0, "DataElementsFromTarget",
"elements_from"], "msg": "Field required", "input": {"destination": {"type": "hdas"},
"items": [{"src": "url", "name": "Reference information", "url": "base64://eyJ0ZXN0IjogInRlc3QifQ==",
"extensionn": "txt"}]}}'
- '{"type": "missing", "loc": ["body", "request_state", "targets", 0, "HdcaDataItemsFromTarget",
"src"], "msg": "Field required", "input": {"destination": {"type": "hdas"}, "items":
[{"src": "url", "name": "Reference information", "url": "base64://eyJ0ZXN0IjogInRlc3QifQ==",
"extensionn": "txt"}]}}'
- '{"type": "literal_error", "loc": ["body", "request_state", "targets", 0, "HdcaDataItemsFromTarget",
"destination", "type"], "msg": "Input should be ''hdca''", "input": "hdas"}'
- '{"type": "missing", "loc": ["body", "request_state", "targets", 0, "HdcaDataItemsFromTarget",
"items_from"], "msg": "Field required", "input": {"destination": {"type": "hdas"},
"items": [{"src": "url", "name": "Reference information", "url": "base64://eyJ0ZXN0IjogInRlc3QifQ==",
"extensionn": "txt"}]}}'
- '{"type": "literal_error", "loc": ["body", "request_state", "targets", 0, "FtpImportTarget",
"destination", "type"], "msg": "Input should be ''hdca''", "input": "hdas"}'
- '{"type": "missing", "loc": ["body", "request_state", "targets", 0, "FtpImportTarget",
"src"], "msg": "Field required", "input": {"destination": {"type": "hdas"}, "items":
[{"src": "url", "name": "Reference information", "url": "base64://eyJ0ZXN0IjogInRlc3QifQ==",
"extensionn": "txt"}]}}'
- '{"type": "missing", "loc": ["body", "request_state", "targets", 0, "FtpImportTarget",
"ftp_path"], "msg": "Field required", "input": {"destination": {"type": "hdas"},
"items": [{"src": "url", "name": "Reference information", "url": "base64://eyJ0ZXN0IjogInRlc3QifQ==",
"extensionn": "txt"}]}}this is getting worse with more complex collections, and it's not just the validation errors, this translates into the schema we could provide for people to author payloads etc. |
|
The way Pydantic displays errors should not be relevant to how we design the API IMO. Also the data fetch schema allows creation of libraries and library folders and reading things from different sources... none of those things have direct equivalents in the workflow run API right? I remain unconvinced this isn't an over correction and over specification to a more limited API - I also remain unconvinced the data fetch schema is inferior. I do however believe that both the data fetch API and the data landing API should support workflow-run style uploads - that just makes sense to me and would enable this use case. I would just add a payload_type property on the those APIs to implement it instead of duplicating the APIs. |
Of course, however usability should matter. I used this to illustrate 2 issues with the current syntax:
which is then why pydantic has to validate everything that thing could be, and I think that's a design issue.
like
I've added |
661bd62 to
4672dd0
Compare
Makes it considerably easier for sites that support data landing and workflow requests, and it's also easier to read and validate. Closes galaxyproject#21097
4672dd0 to
5752c17
Compare
Makes it considerably easier for sites that support data landing and workflow requests, and it's also easier to read and validate.
Closes #21097
Hoping we can get this into 25.1 since it's new functionality that is not yet out in the wild.
How to test the changes?
(Select all options that apply)
License